boot: bootutil: Fix max image size computation for swap-move and swap-offset #2283
Conversation
When computing the maximum image size in bootutil_max_image_size for swap-move or swap-offset strategy, the computation was using the size of the flash area provided as argument and was not taking into account the size of the padding sector. This was causing an incorrect size to be returned in some cases, for example when the two slots have the same size or when the slots haven't the same size but the routine is called for the slot containing the padding sector. For example, let's imagine swap-move is being used on a device having a sector size S and two slots of N bytes. This is valid configuration and the maximum image size is N - S - T, T being the size of the trailer rounded up to the next multiple of S. When calling bootutil_max_image_size with either the primary or secondary slot, the size N - T is returned, which is incorrect. This commit fixes the issue by computing always the maximum image using the size of the slot containing the padding and substracting the size of the padding and of the aligned trailer. Signed-off-by: Thomas Altenbach <[email protected]>
When checking the size of an image in bootutil_img_validate, the offset to the end of the TLV area was used as the image size in all cases. However, when using swap-offset, the upgrade image is written in the secondary with an offset. This offset is not part of the image and must therefore not be taken into account in the image size. Signed-off-by: Thomas Altenbach <[email protected]>
For the swap-move and swap-offset strategies, the computation of the largest image size was not taking taking into account the padding that is needed when using those strategies. Due to this limitation, the simulator is currently using hardcoded image sizes, smaller than the maximum possible size, when running tests for the swap-move or swap-offset strategies. This commit fixes the maximum image size computation for those strategies. Signed-off-by: Thomas Altenbach <[email protected]>
The simulator was testing the upgrade with the largest image possible for all strategies, except for overwrite-only, swap-move and swap-offset because some tests were failing when the maximum image size was used. For overwrite-only, this was due to an incorrect trailer size computation. This has been fixed by 88294be. For swap-move and swap-offset, this was due to the simulator not taking into account the padding needed by those strategies in the primary or secondary slot, but also to incorrect computation of the maximum image size in some cases by the MCUboot library. Both issues have been fixed by the previous commits. Since all those issues have been fixed, the simulator can now be configured to test upgrade with the largest possible image for all strategies. Note that logic needed to generate image of a given image is kept even if not useful anymore at the moment, since that might be needed when test will be added to ensure proper behavior when images of different sizes are used. Signed-off-by: Thomas Altenbach <[email protected]>
taltenbach
force-pushed
the
fix/swap-move-swap-offset-max-img-size
branch
from
a494479 to
52ba928
Compare
There was a problem hiding this comment.
According to my tests, fixing this comparison will reveal another issue - that swap_scramble_trailer_sectors(..) is capable of erasing image in the secondary slot, as it assumes that the trailer is present in both of them.
As a result - the device may become unresponsive after the update is tested.
I am using a bit outdated source code ATM, will try to reproduce it on the main/w PR too.
EDIT: Reproduced on the main branch too.
Steps to reproduce:
- Configure two "efficient" slots.
- Use the application that is close to the size limit.
- Flash it alongside the MCUboot.
- Build a smaller update candidate.
- Trigger the update.
- Neither old nor the new application will boot.
Logs:
*** Booting MCUboot v2.1.0-dev-7bdc0e1ebfd8 ***
*** Using nRF Connect SDK v3.0.99-503b0c832dfa ***
*** Using Zephyr OS v4.1.99-31155f95d8a4 ***
I: Starting bootloader
I: Primary image: magic=unset, swap_type=0x1, copy_done=0x3, image_ok=0x3
I: Secondary image: magic=good, swap_type=0x2, copy_done=0x3, image_ok=0x3
I: Boot source: none
I: Image index: 0, Swap type: test
I: Starting swap using move algorithm.
E: Image in the primary slot is not valid!
E: Unable to find bootable image
*** Booting MCUboot v2.1.0-dev-7bdc0e1ebfd8 ***
*** Using nRF Connect SDK v3.0.99-503b0c832dfa ***
*** Using Zephyr OS v4.1.99-31155f95d8a4 ***
I: Starting bootloader
I: Primary image: magic=good, swap_type=0x2, copy_done=0x1, image_ok=0x3
I: Secondary image: magic=bad, swap_type=0x0, copy_done=0x2, image_ok=0x2
I: Boot source: none
I: Image index: 0, Swap type: revert
E: Image in the secondary slot is not valid!
E: Image in the primary slot is not valid!
E: Unable to find bootable image
The |
|
DNM to investigate issue |
You are using NCS so really the results are invalid anyway, cannot replicate with nrf5340dk using upstream (rebased this branch on main). Original application: Update candidate: Upgrade: And revert: Can see the 2 different images with the |
|
Have tried with an even closer to limit sized original image and can see no problems (swap using move): Though @taltenbach I think the calculations here are just wrong, have tried changing slot sizes but seemingly hitting a wall, with 110 sectors in slot1 I get: and with 109 I get: I need to use 111, which is definitely not right because there is a swap sector, then there is the trailer sector, so 110 should be correct. But anyway, with 111 sectors, I tried again and upgrade was successful: And revert was also successful: So cannot reproduce your problem @tomchy which is likely a downstream issue |
|
I've just reproduced the issue using nRF52840DK with the following setup: Update candidate size: If the difference in slots is just a single page, than there is still memory allocated on the secondary slot for the trailer - you simply optimize the "move"/"scratchpad" area, so the issue is not reproduced. |
|
@tomchy Unless I missed something the layout you're using is not a valid swap-move layout. From the MCUboot design document, in the swap-move section:
The trailer is present in both the primary and secondary slots, so you have to allocate enough space in the secondary slot for the trailer, and that space must be sector-aligned. So, assuming a 0x76000-byte primary slot and 4096-byte sectors, your secondary slot must be 0x75000-byte large. Note in practice you can use a larger secondary slot (e.g. by simplicity some layouts use identical size for the primary and secondary slot) but that is not required and is just wasting flash memory. So, in your case, the size of the secondary slot is the limiting factor and the maximum image size is Perhaps we want Also, I'm a bit surprised So this check is considering the layout is fine: if ((num_sectors_pri != num_sectors_sec) &&
(num_sectors_pri != (num_sectors_sec + 1)) &&
(num_usable_sectors_pri != (num_sectors_sec + 1))) {
BOOT_LOG_WRN("Cannot upgrade: not a compatible amount of sectors");
BOOT_LOG_DBG("slot0 sectors: %d, slot1 sectors: %d, usable slot0 sectors: %d",
(int)num_sectors_pri, (int)num_sectors_sec,
(int)(num_usable_sectors_pri - 1));
return 0;
} else if (num_sectors_pri > BOOT_MAX_IMG_SECTORS) {
BOOT_LOG_WRN("Cannot upgrade: more sectors than allowed");
return 0;
}@nordicjm Isn't |
|
@nordicjm Regarding:
If your primary slot has 112 sectors and the trailer fits in a single sector, then I don't understand why having a secondary slot of 110 sectors should be correct since you need the trailer sector also in the secondary slot:
The trailer of the secondary slot is not used for storing swap status during the upgrade/revert but is still needed for marking the upgrade as pending. |
It should be putting that after the image trailer (not the update progress trailer) without a gap (unlike in primary, where there is a gap), no? Though actually I guess you are right either way because if the image reverts then you will be placing that status in the primary slot which now contains the secondary slot image, so you need to include that in the size of the secondary slot so yes, primary slot should always be the same size or exactly 1 sector more for swap using move mode. |
When computing the maximum image size in
bootutil_max_image_sizefor the swap-move or swap-offset strategy, the computation was using the size of the flash area provided as argument and was not taking into account the size of the padding sector. This was causing an incorrect (too large) size to be returned in the following cases:For example, let's imagine swap-move is being used on a device having a sector size
Sand two slots ofNbytes. This is valid configuration and the maximum image size isN - S - T,Tbeing the size of the trailer rounded up to the next multiple ofS. When callingbootutil_max_image_sizewith either the primary or secondary slot, the sizeN - Tis returned, which is incorrect.This was not detected by the tests since, due to limitations of the simulator, the swap-move and swap-offset strategies were not tested with the largest possible images. In addition to fixing the issue in
bootutil_max_image_size, this PR modifies the simulator to allow the simulator to use largest possible images for all strategies and therefore detect those kind of issues.